Refactor `range`/`gen_series` signature away from user defined by Jefffrey · Pull Request #18317 · apache/datafusion

Jefffrey · 2025-10-28T07:39:28Z

Which issue does this PR close?

Closes Internal error with generate_series: Internal error: could not cast array of type Date32 #15881
- See my notes below

Rationale for this change

Trying to move away from user defined signatures where possible; mainly to ensure consistency of error checking/messages. The original issue is because the function has to do this checking itself leading to inconsistency of error used (ideally shouldn't be internal). By uplifting away from a user defined signature we can make use of existing code meant to handle this checking and error messages for us.

What changes are included in this PR?

Defined range/generate_series signature via coercible API instead of being user defined. Some accompanying changes are needed in the signature code to make this possible.

Are these changes tested?

Added SLT tests and fixed any existing ones.

Are there any user-facing changes?

No (error messages do change though)

Jefffrey · 2025-10-28T07:40:49Z

            TypeSignatureClass::Binary if native_type.is_binary() => {
                Ok(origin_type.to_owned())
            }
+            _ if native_type.is_null() => Ok(origin_type.to_owned()),


We were missing this even though we check for it in matches_native_type; I think this will allow passing null types to coercible signatures without needing to explicitly specify null type handling

Jefffrey · 2025-10-28T07:41:01Z

 }

 impl Range {
+    fn defined_signature() -> Signature {


This is the main change

Jefffrey · 2025-10-28T07:42:15Z

+        // Signature can only guarantee we get a date type, not specifically
+        // date32 so handle potential cast from date64 here.
+        let start = cast(start, &DataType::Date32)?;
+        let start = as_date32_array(&start)?;
+        let stop = cast(stop, &DataType::Date32)?;
+        let stop = as_date32_array(&stop)?;


I don't see much benefit in handling date64 natively anyway (given it's a bit of a weird type) so I think this is fine

Jefffrey · 2025-10-28T07:43:59Z

+        // Signature can only guarantee we get a timestamp type, not specifically
+        // timestamp(ns) so handle potential cast from other timestamps here.
+        fn cast_to_ns(arr: &ArrayRef) -> Result<ArrayRef> {
+            match arr.data_type() {
+                DataType::Timestamp(TimeUnit::Nanosecond, _) => Ok(Arc::clone(arr)),
+                DataType::Timestamp(_, tz) => Ok(cast(
+                    arr,
+                    &DataType::Timestamp(TimeUnit::Nanosecond, tz.to_owned()),
+                )?),
+                _ => unreachable!(),
+            }
+        }
+        let start = cast_to_ns(start)?;
+        let start = as_timestamp_nanosecond_array(&start)?;
+        let stop = cast_to_ns(stop)?;
+        let stop = as_timestamp_nanosecond_array(&stop)?;


I wonder if is better to instead try uplift the signature coercible API to allow specifying we only want timestamp(ns) (of any timezone) and let that handle coercion for us 🤔

I agree that would be nicer but I think it could also be done as a follow on PR

Jefffrey · 2025-10-28T07:44:23Z

+       range(arrow_cast(1, 'Int8'), 5, -1),
+       range(arrow_cast(1, 'Int16'), arrow_cast(-5, 'Int8'), 1),
+       range(arrow_cast(1, 'Int32'), arrow_cast(-5, 'Int16'), arrow_cast(-1, 'Int8')),
+       range(DATE '1992-09-01', DATE '1993-03-01', arrow_cast('1 MONTH', 'Interval(YearMonth)')),
+       range(DATE '1993-02-01', arrow_cast(DATE '1993-01-01', 'Date64'), INTERVAL '-1' DAY),
+       range(arrow_cast(DATE '1989-04-01', 'Date64'), DATE '1993-03-01', INTERVAL '1' YEAR),
+       range(arrow_cast(DATE '1993-03-01', 'Date64'), arrow_cast(DATE '1989-04-01', 'Date64'), INTERVAL '1' YEAR)


Uplift tests to ensure coercion is handled properly for different types

I recommend making this more explicit by:

Revert the change to the existing tests

Add a new query right below this one that adds the casts to the other integer types

Jefffrey · 2025-10-28T07:45:52Z

-query error DataFusion error: Internal error: Unexpected argument type for generate_series : Date32
+query error DataFusion error: Error during planning: Internal error: Function 'generate_series' failed to match any signature
 select generate_series(arrow_cast('2021-01-01T00:00:00', 'Timestamp(Nanosecond, Some("-05:00"))'), DATE '2021-01-02', INTERVAL '1' HOUR);

+## mixing types not allowed even if an argument is null
+query error DataFusion error: Error during planning: Internal error: Function 'generate_series' failed to match any signature
+select generate_series(TIMESTAMP '1992-09-01', DATE '1993-03-01', NULL);
+
+query error DataFusion error: Error during planning: Internal error: Function 'generate_series' failed to match any signature
+select generate_series(1, '2024-01-01', '2025-01-02');
+
+query error DataFusion error: Error during planning: Internal error: Function 'generate_series' failed to match any signature
+select generate_series('2024-01-01'::timestamp, '2025-01-02', interval '1 day');


So these are the original examples from the issue; technically we still return an internal error, so it feels a bit wrong to claim we close #15881 (well its an internal error nested in a plan error apparently)

However this internal error comes from the function signature related code, so we can fix it there and it should propagate to here (aka its not a specific issue with generate_series)

Yes, I agree we should fix it in the signature related code

I think it would also be ideal if we can provide more details on what went wrong and how we can fix it (aka as a user who doesn't understand the internal implementation, if I get this error message, how do I fix it?)

alamb

Thanks @Jefffrey -- I will try and make it through this PR later today

alamb · 2025-10-29T12:39:39Z

    };
 }

+macro_rules! singleton_variant {


Could you potentially add some documentation here about what is different about this macro compared to the others in this module?

alamb

Thank you @Jefffrey -- this looks like an improvement to me -- I had a few suggestions and I think we should try and improve the messages even more, but we can do that as a follow on PR

alamb · 2025-10-29T19:22:56Z

+
+        // Signature can only guarantee we get a timestamp type, not specifically
+        // timestamp(ns) so handle potential cast from other timestamps here.
+        fn cast_to_ns(arr: &ArrayRef) -> Result<ArrayRef> {


given cast already checks for the same type, I wonder if this is necessary? Or is the idea to cast to nanoseconds and maintain the timezone?

Yeah it's to cast from potentially other timestamp time units down to nanoseconds

alamb · 2025-10-29T19:23:20Z

+        // Signature can only guarantee we get a timestamp type, not specifically
+        // timestamp(ns) so handle potential cast from other timestamps here.
+        fn cast_to_ns(arr: &ArrayRef) -> Result<ArrayRef> {
+            match arr.data_type() {
+                DataType::Timestamp(TimeUnit::Nanosecond, _) => Ok(Arc::clone(arr)),
+                DataType::Timestamp(_, tz) => Ok(cast(
+                    arr,
+                    &DataType::Timestamp(TimeUnit::Nanosecond, tz.to_owned()),
+                )?),
+                _ => unreachable!(),
+            }
+        }
+        let start = cast_to_ns(start)?;
+        let start = as_timestamp_nanosecond_array(&start)?;
+        let stop = cast_to_ns(stop)?;
+        let stop = as_timestamp_nanosecond_array(&stop)?;


I agree that would be nicer but I think it could also be done as a follow on PR

alamb · 2025-10-29T19:24:24Z

+       range(arrow_cast(1, 'Int8'), 5, -1),
+       range(arrow_cast(1, 'Int16'), arrow_cast(-5, 'Int8'), 1),
+       range(arrow_cast(1, 'Int32'), arrow_cast(-5, 'Int16'), arrow_cast(-1, 'Int8')),
+       range(DATE '1992-09-01', DATE '1993-03-01', arrow_cast('1 MONTH', 'Interval(YearMonth)')),
+       range(DATE '1993-02-01', arrow_cast(DATE '1993-01-01', 'Date64'), INTERVAL '-1' DAY),
+       range(arrow_cast(DATE '1989-04-01', 'Date64'), DATE '1993-03-01', INTERVAL '1' YEAR),
+       range(arrow_cast(DATE '1993-03-01', 'Date64'), arrow_cast(DATE '1989-04-01', 'Date64'), INTERVAL '1' YEAR)


I recommend making this more explicit by:

Revert the change to the existing tests

Add a new query right below this one that adds the casts to the other integer types

alamb · 2025-10-29T19:26:13Z

-query error DataFusion error: Internal error: Unexpected argument type for generate_series : Date32
+query error DataFusion error: Error during planning: Internal error: Function 'generate_series' failed to match any signature
 select generate_series(arrow_cast('2021-01-01T00:00:00', 'Timestamp(Nanosecond, Some("-05:00"))'), DATE '2021-01-02', INTERVAL '1' HOUR);

+## mixing types not allowed even if an argument is null
+query error DataFusion error: Error during planning: Internal error: Function 'generate_series' failed to match any signature
+select generate_series(TIMESTAMP '1992-09-01', DATE '1993-03-01', NULL);
+
+query error DataFusion error: Error during planning: Internal error: Function 'generate_series' failed to match any signature
+select generate_series(1, '2024-01-01', '2025-01-02');
+
+query error DataFusion error: Error during planning: Internal error: Function 'generate_series' failed to match any signature
+select generate_series('2024-01-01'::timestamp, '2025-01-02', interval '1 day');


Yes, I agree we should fix it in the signature related code

I think it would also be ideal if we can provide more details on what went wrong and how we can fix it (aka as a user who doesn't understand the internal implementation, if I get this error message, how do I fix it?)

…e#18317) ## Which issue does this PR close?  - Closes apache#15881 - See my notes below ## Rationale for this change  Trying to move away from user defined signatures where possible; mainly to ensure consistency of error checking/messages. The original issue is because the function has to do this checking itself leading to inconsistency of error used (ideally shouldn't be internal). By uplifting away from a user defined signature we can make use of existing code meant to handle this checking and error messages for us. ## What changes are included in this PR?  Defined range/generate_series signature via coercible API instead of being user defined. Some accompanying changes are needed in the signature code to make this possible. ## Are these changes tested?  Added SLT tests and fixed any existing ones. ## Are there any user-facing changes?  No (error messages do change though)

Refactor range/gen_series signature away from user defined

50d9678

github-actions Bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Oct 28, 2025

Jefffrey commented Oct 28, 2025

View reviewed changes

Jefffrey marked this pull request as ready for review October 28, 2025 08:27

alamb reviewed Oct 29, 2025

View reviewed changes

alamb approved these changes Oct 29, 2025

View reviewed changes

Jefffrey added 3 commits October 30, 2025 17:03

Merge branch 'main' into gen-series-signature

4806623

review comments

90932de

I am very good at the English language

f0de14a

alamb added this pull request to the merge queue Oct 30, 2025

Merged via the queue into apache:main with commit 67550f1 Oct 30, 2025
32 checks passed

Jefffrey deleted the gen-series-signature branch October 31, 2025 02:16

Jefffrey mentioned this pull request Nov 12, 2025

Handle Null type in TypeSignatureClass more consistently #15013

Closed

Conversation

Jefffrey commented Oct 28, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Jefffrey Oct 30, 2025 •

edited

Loading